Adds installation tests for conda/uv/modularized installation#5563
Adds installation tests for conda/uv/modularized installation#5563kellyguo11 wants to merge 6 commits intoisaac-sim:developfrom
Conversation
Align the kitless OpenUSD dependency with Kit 110.1.1 so installs resolve the matching USD runtime. Co-authored-by: Cursor <cursoragent@cursor.com>
Add coverage for training workflow installs and conda-based install CI so the installation paths are exercised separately from the usd-core dependency bump. Co-authored-by: Cursor <cursoragent@cursor.com>
kellyguo11
requested review from
Mayankm96,
hhansen-bdai,
matthewtrepte,
ossamaAhmed and
pascal-roth
as code owners
Greptile SummaryThis PR adds automated CI for conda and uv installation paths, introduces a new
Confidence Score: 3/5The conda CI job's outer timeout equals its inner Docker run timeout, meaning build overhead will predictably cut the Docker run short on every run. The conda job sets
Important Files Changed
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Changes as changes job
participant UV as install-tests (uv)
participant Conda as install-tests-conda
participant Runner as run_install_ci.py
participant Docker as Docker
GHA->>Changes: trigger (always)
Changes-->>GHA: "run_install_tests=true (always)"
GHA->>UV: start (needs: changes)
UV->>Runner: "docker --gpu -- --tb=short -sv -m uv"
Runner->>Docker: build Dockerfile.installci (uv image)
Docker-->>Runner: uv_image_tag
Runner->>Docker: "run uv image (timeout=5400s)"
Docker-->>Runner: pytest results (uv tests only)
Runner-->>UV: exit code
GHA->>Conda: start (needs: changes)
Conda->>Runner: "docker --conda --gpu -- --tb=short -sv -m conda"
Runner->>Docker: build Dockerfile.installci (uv base)
Docker-->>Runner: uv_image_tag
Runner->>Docker: "build Dockerfile.installci-conda (UV_IMAGE=uv_image_tag)"
Docker-->>Runner: conda_image_tag
Runner->>Docker: "run conda image (timeout=7200s)"
Docker-->>Runner: pytest results (conda tests only)
Runner-->>Conda: exit code
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if: needs.changes.outputs.run_install_tests == 'true' | ||
| runs-on: [self-hosted, gpu] | ||
| timeout-minutes: 90 | ||
| timeout-minutes: 120 |
There was a problem hiding this comment.
Both install-tests and install-tests-conda are set to timeout-minutes: 120. For the conda job, run_install_ci.py sets an inner Docker run timeout of 7200 s (exactly 120 minutes). Every second spent on actions/checkout, building the uv base image, and then building the conda image on top of it eats into that same 120-minute budget — meaning the actual Docker run will be cancelled by GitHub Actions before the inner timeout ever fires. In practice even a fast build (~5–10 minutes) leaves the Docker run with only ~110 minutes instead of 120, and the GitHub job is the one that kills the container without a clean JUnit report. Consider raising this to at least timeout-minutes: 150 (or 160) to provide a genuine buffer for build overhead.
| # Prepend the two internal registries so they are tried before pypi.org. | ||
| for url in _INTERNAL_ISAACSIM_INDEX_URLS: | ||
| if url not in extra_index_urls: | ||
| extra_index_urls.insert(0, url) |
There was a problem hiding this comment.
insert(0, url) loop reverses the intended order of internal registries
The loop inserts each URL from _INTERNAL_ISAACSIM_INDEX_URLS at position 0 one at a time. After both inserts the list is [ct-omniverse-pypi, sw-isaacsim-pypi, NVIDIA_INDEX_URL, ...] — the opposite of the declaration order in _INTERNAL_ISAACSIM_INDEX_URLS (where sw-isaacsim-pypi is listed first). With uv's unsafe-best-match strategy, index ordering can affect resolution. Replace the loop with a slice-prepend to preserve the declared order.
| # Prepend the two internal registries so they are tried before pypi.org. | |
| for url in _INTERNAL_ISAACSIM_INDEX_URLS: | |
| if url not in extra_index_urls: | |
| extra_index_urls.insert(0, url) | |
| # Prepend the two internal registries so they are tried before pypi.org. | |
| prepend = [url for url in _INTERNAL_ISAACSIM_INDEX_URLS if url not in extra_index_urls] | |
| extra_index_urls = prepend + extra_index_urls |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds automated installation workflow tests for conda/uv/modularized paths (7 test combinations), a conda-enabled Docker image, env var overrides for internal Isaac Sim builds in the install CLI, and makes PhysX/OvPhysX imports optional in the cartpole task config to support kitless (Newton-only) installs. It also changes SimulationCfg.physics_material and TerrainImporterCfg.physics_material from RigidBodyMaterialCfg to RigidBodyMaterialBaseCfg for solver-agnostic defaults.
Design Assessment
Acceptable, but the conditional import pattern needs consistency. The RigidBodyMaterialBaseCfg refactor is clean — it's the correct solver-agnostic abstraction and backward-compatible via the existing __getattr__ lazy-forwarding shim. The module-level class mutation pattern (CartpolePhysicsCfg.physx = PhysxCfg()) works correctly with the preset system's _preset_fields() which explicitly scans vars(cls). However, only cartpole_env_cfg.py gets the conditional import treatment — other task configs (e.g. humanoid) still have unconditional PhysX imports and will break on kitless installs.
Findings
Details in inline comments.
Test Coverage
- New code (install workflow tests): Yes — 7 E2E combinations covering uv/conda × kitless/pip/source
install.pyenv var logic: No — complex URL-building and version-spec parsing has no unit testscartpole_env_cfg.pyfallback: Implicit only — kitless E2E tests exercise the happy path but no negative-path tests for requesting unavailable presets- Type change (
RigidBodyMaterialBaseCfg): No dedicated test — backward-compatible via inheritance, low risk
CI Status
- Build Wheel: ✅ pass
- Check changelog fragments: ❌ fail
- Installation Tests (uv): ⏳ pending
- Installation Tests (conda): ⏳ pending
- pre-commit: ⏳ pending
Verdict
Minor fixes needed
The overall approach is sound — modularized install support is valuable, and the test matrix is comprehensive. The RigidBodyMaterialBaseCfg change is clean. Address the warning-level items (asymmetric import guarding, URL insertion order) and consider adding unit tests for _install_isaacsim() env var logic. The changelog fragment CI failure also needs attention.
| from isaaclab_ovphysx.physics import OvPhysxCfg | ||
| from isaaclab_physx.physics import PhysxCfg | ||
|
|
||
| try: |
There was a problem hiding this comment.
🟡 Warning: Asymmetric import guarding — isaaclab_newton (line 8) is imported unconditionally while isaaclab_ovphysx and isaaclab_physx are guarded with try/except.
If the goal is to support modularized installs, this means an install with PhysX-only (no Newton) would crash on import of the cartpole env. If Newton is intentionally a hard dependency for all kitless installs, a brief comment documenting that assumption would help future maintainers.
Also worth noting: this is the only task config with conditional imports — other envs (humanoid, etc.) still import isaaclab_physx unconditionally and will fail on kitless installs.
| use_cuda_graph=True, | ||
| ) | ||
| ovphysx: OvPhysxCfg = OvPhysxCfg() | ||
|
|
There was a problem hiding this comment.
🟡 Warning: Preset names silently vanish when backends are unavailable — When PhysxCfg is None, CartpolePhysicsCfg.physx is never created, and when OvPhysxCfg is None, .ovphysx is never created. A user requesting presets=physx on a kitless install gets a generic "Unknown preset 'physx'" error from the preset system rather than a clear "isaaclab_physx is not installed" message.
This works correctly (not a bug), but the error UX could be improved. Consider adding a comment or, better, a descriptive error path in the preset resolver for known-but-unavailable backends.
| version_spec = os.environ.get("ISAACSIM_VERSION_SPEC", ISAACSIM_VERSION_SPEC) | ||
| extras = os.environ.get("ISAACSIM_EXTRAS", ISAACSIM_EXTRAS) | ||
| use_pre = os.environ.get("ISAACSIM_USE_PRE", "").strip().lower() in ("1", "true", "yes") | ||
|
|
There was a problem hiding this comment.
🔵 Suggestion: insert(0, ...) in a loop reverses the declaration order — Iterating _INTERNAL_ISAACSIM_INDEX_URLS and calling insert(0, url) each time produces [ct-omniverse, sw-isaacsim, NVIDIA] instead of [sw-isaacsim, ct-omniverse, NVIDIA].
Practically this doesn't matter (pip resolves across all indexes regardless of --extra-index-url order), but if declaration order was intentional, replace the loop with:
| # Prepend the two internal registries so they are tried before pypi.org. | |
| for url in reversed(_INTERNAL_ISAACSIM_INDEX_URLS): | |
| if url not in extra_index_urls: | |
| extra_index_urls.insert(0, url) |
1 participant
Description
Adds automated tests for several installation paths, including using conda and uv environments and minimal installation of modularized packages.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there